Skip to content

Fix name collisions in ArduinoISP sketch #3

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 3, 2020
Merged

Fix name collisions in ArduinoISP sketch #3

merged 2 commits into from
Sep 3, 2020

Conversation

per1234
Copy link
Contributor

@per1234 per1234 commented Sep 3, 2020

Name collisions caused compilation of the ArduinoISP sketch to fail when:

@per1234 per1234 added the type: imperfection Perceived defect in any part of project label Sep 3, 2020
…bitbanged SPI mode for board using ArduinoCore-API

When using bit banged SPI, which the sketch did when compiled for any architecture other than AVR, a `SPISettings` class was declared by the sketch. At the time the sketch was written, it was reasonable to expect this would not cause a name collision, since SPI.h is not `#include`d when doing bit banged SPI. However, since then a `SPISettings` class has been declared in [ArduinoCore-API's HardwareSPI.h](https://github.com/arduino/ArduinoCore-API/blob/932c7c7d4d4d334b10484284cc846672ad59607c/api/HardwareSPI.h#L37), causing the ArduinoISP sketch to not compile for any board whose core uses ArduinoCoreAPI (currently Arduino Mega AVR Boards, "Arduino nRF528x Boards (Mbed OS]", and "Arduino Mbed OS Boards (nRF52840 / STM32H747)"):

```
  /github/workspace/examples/11.ArduinoISP/ArduinoISP/ArduinoISP.ino:191:27: error: reference to 'SPISettings' is ambiguous
       void beginTransaction(SPISettings settings) {
                             ^~~~~~~~~~~
  /github/workspace/examples/11.ArduinoISP/ArduinoISP/ArduinoISP.ino:167:7: note: candidates are: class SPISettings
   class SPISettings {
         ^~~~~~~~~~~
  In file included from /github/home/.arduino15/packages/arduino/hardware/megaavr/1.8.6/cores/arduino/api/ArduinoAPI.h:31:0,
                   from /github/home/.arduino15/packages/arduino/hardware/megaavr/1.8.6/cores/arduino/Arduino.h:23,
                   from /github/workspace/examples/11.ArduinoISP/ArduinoISP/ArduinoISP.ino:39:
  /github/home/.arduino15/packages/arduino/hardware/megaavr/1.8.6/cores/arduino/api/HardwareSPI.h:37:7: note:                 class arduino::SPISettings
   class SPISettings {
         ^~~~~~~~~~~
```

The fix is to use the `ARDUINO_API_VERSION` macro defined by ArduinoCore-API to detect when it is in use and make the bitbanged SPI code use the `SPISettings` class from ArduinoCore-API in this case.
…d OS

The name collision between the variable named "error" in the ArduinoISP sketch and a function of that name declared by Mbed OS causes compilation of the sketch to fail for boards that use the "Arduino nRF528x Boards (Mbed OS)" or "Arduino Mbed OS Boards (nRF52840 / STM32H747)" cores.
Copy link
Contributor

@ubidefeo ubidefeo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, @per1234
One question: I saw you removed the friend class directive, why was it there?

@per1234
Copy link
Contributor Author

per1234 commented Sep 3, 2020

I saw you removed the friend class directive, why was it there?

Previously, BitBangedSPI::beginTransaction() was directly referencing the private member SPISettings::clock, which required friendship. The SPISettings class in ArduinoCore-API provides a getClockFreq() getter for this purpose:
https://github.com/arduino/ArduinoCore-API/blob/45e4e5aba6ad1a5b67026b686099cc0fce437cdc/api/HardwareSPI.h#L73
and even provides guidance for referencing those private members:

// Core developer MUST use an helper function in beginTransaction() to use this data

So I just mirrored the way the SPISettings class from ArduinoCore-API was written in the SPISettings class declared in ArduinoISP that is used when the board being compiled for doesn't use ArduinoCoreAPI (and thus doesn't have the ARDUINO_API_VERSION macro defined). This way, SPISettings provides a consistent API to BitBangedSPI either way.

I'm definitely open to feedback on this. I'm not an expert in this subject, and only took on the project because the alternative to fixing the compilation failure was excluding the ArduinoISP sketch from the CI, which very much didn't sit well with me.

@ubidefeo
Copy link
Contributor

ubidefeo commented Sep 3, 2020

Previously, BitBangedSPI::beginTransaction() was directly referencing the private member SPISettings::clock

right! I never do that, always loved getter/setter strategies

as I said, this looks well written

@per1234 per1234 merged commit f350ea7 into arduino:main Sep 3, 2020
@per1234 per1234 deleted the fix-arduinoisp-name-collisions branch September 3, 2020 10:00
@per1234 per1234 added type: imperfection Perceived defect in any part of project and removed type: imperfection Perceived defect in any part of project labels Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants